-
Notifications
You must be signed in to change notification settings - Fork 2.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Reset category and tag on navigating to confirmation page #33222
Conversation
Reviewer Checklist
Screenshots/VideosAndroid: Nativeandroid-native-2023-12-18_14.50.29.mp4Android: mWeb Chromeandroid-chrome-2023-12-18_14.46.15.mp4iOS: Nativeios-native-2023-12-18_14.54.23.mp4iOS: mWeb Safariios-safari-2023-12-18_14.51.54.mp4MacOS: Chrome / Safaridesktop-chrome-2023-12-18_14.38.03.mp4MacOS: Desktopdesktop-app-2023-12-18_14.40.26.mp4 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would prefer if we added a unit test for this. Ideally whenever we fix a bug we should add a unit test to prevent a regression. @FitseTLT can you look into writing a unit test for this fix?
@blimpich Is the change related to this pr suitable for unit test? It's not like this pr introduces the tag and category reset functions. Can U please give me a bit of elaboration? |
@blimpich Ok now, working on creating the test. 👍 |
@FitseTLT Unit tests aren't just for new functionality, they are also for catching bugs 🐛. In a perfect world this bug should have never happened because a unit test should have caught it. If adding a test to this only takes you 2-3 hours I think it would be well-worth adding it. If it's going to take longer than that then I think we can go ahead and merge this without adding a new unit test. Thank you for looking into it! 🙂 |
@blimpich I couldn't get a perfect example unit test similar to what I am supposed to create for this pr. What I tried is set tag and category in onyx to a draft transaction and then render the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good @FitseTLT, thank you for trying. Yes historically we rely on E2E tests for testing these kinds of changes. Unit tests, though, are much cheaper and quicker than E2E testing, so I'm hoping that in the future we we'll start writing more unit tests.
I asked you largely because I wanted someone to investigate whether it would be easy to add a unit test for this component, which it looks like it isn't. Thank you for investigating that. I appreciate your work. Approved!
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
🚀 Deployed to staging by https://github.com/blimpich in version: 1.4.15-0 🚀
|
🚀 Deployed to production by https://github.com/jasperhuangg in version: 1.4.15-5 🚀
|
Details
Fixed Issues
$ #33067
PROPOSAL: #33067 (comment)
Tests
Offline tests
QA Steps
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)myBool && <MyComponent />
.src/languages/*
files and using the translation methodWaiting for Copy
label for a copy review on the original GH to get the correct copy.STYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)ScrollView
component to make it scrollable when more elements are added to the page.main
branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTest
steps.Screenshots/Videos
Android: Native
android.native.mp4
Android: mWeb Chrome
android.web.mp4
iOS: Native
ios.native.mp4
iOS: mWeb Safari
ios.web.mp4
MacOS: Chrome / Safari
web.mp4
MacOS: Desktop
desktop.mp4